Add task list partition config #6343
Conversation
Shaddoll
requested review from
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
Shaddoll
force-pushed
the
schema
branch
5 times, most recently
from
e87850b
to
9fa4083
Compare
Shaddoll
changed the title
| CREATE TYPE task_list ( | ||
| domain_id uuid, | ||
| name text, | ||
| type int, -- enum TaskRowType {ActivityTask, DecisionTask} | ||
| ack_level bigint, -- task_id of the last acknowledged message | ||
| kind int, -- enum TaskListKind {Normal, Sticky} | ||
| last_updated timestamp | ||
| last_updated timestamp, | ||
| adaptive_partition_config frozen<task_list_partition_config> |
There was a problem hiding this comment.
the use of the word adaptive here precludes it for being used in other contexts. What do you think about just partition_config in case it isn't set to be adaptive for some reason?
There was a problem hiding this comment.
I don't want to confuse this config with the partition_config for zonal isolation.
| @@ -66,6 +66,11 @@ func TestSelectTaskList(t *testing.T) { | |||
| (*tlDB)["ack_level"] = int64(1000) | |||
| (*tlDB)["kind"] = 2 | |||
| (*tlDB)["last_updated"] = now | |||
| (*tlDB)["adaptive_partition_config"] = map[string]interface{}{ | |||
There was a problem hiding this comment.
Nonblocking/Nit: It might be be worth doing a round-trip test on the to and from mapping functions to ensure that no fields get dropped in the future.
| CREATE TYPE task_list ( | ||
| domain_id uuid, | ||
| name text, | ||
| type int, -- enum TaskRowType {ActivityTask, DecisionTask} | ||
| ack_level bigint, -- task_id of the last acknowledged message | ||
| kind int, -- enum TaskListKind {Normal, Sticky} | ||
| last_updated timestamp | ||
| last_updated timestamp, | ||
| adaptive_partition_config frozen<task_list_partition_config> |
There was a problem hiding this comment.
I think we should make this a byte array representing a thrift serialized blob. This way further iterations on this adaptive_partition_config (e.g. adding zone info or other metadata) will not require DB schema change
There was a problem hiding this comment.
My argument is if we want to use serialized blob, we should do it for task_list and task type. And that's what temporal does.
There was a problem hiding this comment.
I think any future structs like this (no-need-to-query-fields) that we introduce going forward should be modeled as binary blob for flexibility purposes, therefore no more new DB types. Migrating away from existing DB types to blob approach is not P0 in my mind. We can consider a major schema at some point and that would the good time to get model such structs as blob.
However if you think extending this DB type with a few more fields in the near future is not going to be too much work (including the schema rollout) then let's go ahead with this.
| _, ok = err.(*p.ConditionFailedError) | ||
| s.True(ok) |
There was a problem hiding this comment.
these 3 lines can be replaces by s.ErrorAs
| @@ -260,13 +260,21 @@ CREATE TYPE task ( | |||
| partition_config map<text, text> | |||
| ); | |||
|
|
|||
| CREATE TYPE task_list_partition_config ( | |||
There was a problem hiding this comment.
@natemort can you check this type and give feedback on what fields you would need for upcoming zonal isolation improvements?
It's better to avoid extra schema changes but if we don't know the shape of it then it's fine. We can come back and change this later.
There was a problem hiding this comment.
Discussed with Zijian, this approach is fine and isn't incompatible. We'll make additional changes in the future since things are a little up in the air still.
Detailed Description
Impact Analysis
Testing Plan
Rollout Plan